Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove unncessary wrapper #1916

Merged
merged 1 commit into from
Jan 2, 2025
Merged

Conversation

ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Dec 18, 2024

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/6325

Description (What does it do?)

This PR removes an unnecessary wrapper around an icon. Previously the wrapping div had an aria-label; this is technically disalllowed and causes some accessibility checkers to issue a warning.

Now the aria-label is on an svg, which is 👍 .

Screenshots

Small cards with a certificate are "affected", but there is no visual change:

Screenshot 2024-12-18 at 3 28 52 PM

How can this be tested?

  1. View a small card with a certificate on RC. These can be found (possibly) in your dashboard, or in the "Similar Resources" carousels inside the learning resource drawer. Check the certificate icon dimensions:
    • svg is 12px by 12px
    • so is the div wrapping it
  2. Do same locally; svg should still be 12px by 12px. The div wrapping it is gone. The SVG itself now has aria-label.
  3. Additionally / Optionally, you could check that an accessibility checker like axe devtools no longer flags the aria-label as invalid. (Run it with the drawer open).

@ChristopherChudzicki ChristopherChudzicki added the Needs Review An open Pull Request that is ready for review label Dec 18, 2024
@ChristopherChudzicki
Copy link
Contributor Author

The JS tests will pass after

The Python test failure is flakiness, i guess.

@ChristopherChudzicki ChristopherChudzicki marked this pull request as ready for review December 20, 2024 19:51
@shanbady shanbady self-requested a review December 23, 2024 14:16
Copy link
Contributor

@shanbady shanbady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@shanbady shanbady added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Dec 23, 2024
@ChristopherChudzicki ChristopherChudzicki merged commit 006e965 into main Jan 2, 2025
11 checks passed
This was referenced Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants